feat(config:install): improve destination directory detection#48
Open
miguelsanchez-upsun wants to merge 10 commits into
Open
feat(config:install): improve destination directory detection#48miguelsanchez-upsun wants to merge 10 commits into
miguelsanchez-upsun wants to merge 10 commits into
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Improves config:install destination directory selection for alt config/executable installs and aligns interactive behavior with --no-interaction / -y.
Changes:
- Honor
XDG_CONFIG_HOMEwhen computing the alt config directory. - Rework
FindBinDir()with an expanded per-OS allowlist, writability checks, and preference to co-locate with the running executable (incl. symlink resolution). - Make
config:installconfirmation prompt respectno-interaction(so-y/--yesskips it).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| internal/config/alt/fs.go | Adds XDG handling for config dir; overhauls bin-dir selection with allowlist, writability probing, and executable co-location logic. |
| internal/config/alt/fs_test.go | Refactors and significantly expands bin-dir selection tests; updates config-dir tests for new resolution behavior. |
| commands/config_install.go | Skips confirmation prompt when no-interaction is enabled (including via -y/--yes). |
Comments suppressed due to low confidence (1)
commands/config_install.go:110
- This change makes
-y/--yes(viano-interaction) skip the confirmation prompt, butcommands/config_install_test.godoesn't currently exercise the interactive-confirmation path (most test environments have non-interactive stdin). Consider adding a unit test that forcesterminal.Stdinto interactive and verifies that the prompt is skipped whenviper.GetBool("no-interaction")is true (may require injecting/stubbing the confirmation function for testability).
if terminal.Stdin.IsInteractive() && !viper.GetBool("no-interaction") {
if !terminal.AskConfirmation("Are you sure you want to continue?", true) {
os.Exit(1)
}
cmd.PrintErrln()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Hm I think actually we should never be saving our own binaries to the HomeBrew path: it should be owned entirely via HomeBrew. |
Move the running-executable match into an exeMatcher closure so the loop in FindBinDir reads as one straightforward pass: skip if not on PATH or not writable, return on co-location, otherwise track first-valid for the fallback. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the temp-file probe in isWritableDir with golang.org/x/sys/unix Access on Unix, falling back to the probe file only on Windows where no equivalent syscall is available. The temp file was created and deleted on every config:install invocation across each allowlist entry; the permission-bit check has the same semantics without the side effect. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Remove /opt/homebrew/bin (macOS) and /home/linuxbrew/.linuxbrew/bin (Linux) from the bin-directory allowlist. Those directories belong to Homebrew and should not be written to by config:install — when the running CLI was itself installed via Homebrew, the alt now lands in /usr/local/bin or ~/.local/bin instead. The symlink-resolution branch in exeMatcher is kept: a Cellar-style layout is still reachable when the user opts in via XDG_BIN_HOME, and the existing test exercises that path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
XDG_BIN_HOME./opt/homebrew/binon macOS,/home/linuxbrew/.linuxbrew/binon Linux): a user who installed via Homebrew gets the alt in/usr/local/binor~/.local/bininstead of polluting the brew prefix.XDG_CONFIG_HOMEwhen locating the config directory (previously ignored on macOS and Windows), even if the target does not yet exist.-yskip the confirmation prompt (previously neither-ynor--no-interactiondid — the prompt only checked stdin TTY).access(2)for the bin-dir writability check on Unix instead of probing with a temp file.